Skip to content

refactor: Separate adding/removing cycles from refunding/consuming cycles#9717

Merged
mraszyk merged 4 commits intodfinity:masterfrom
dsarlis:dimitris/add-remove-cycles
Apr 7, 2026
Merged

refactor: Separate adding/removing cycles from refunding/consuming cycles#9717
mraszyk merged 4 commits intodfinity:masterfrom
dsarlis:dimitris/add-remove-cycles

Conversation

@dsarlis
Copy link
Copy Markdown
Contributor

@dsarlis dsarlis commented Apr 2, 2026

This PR refactors how cycles changes to balance and metrics are happening via the SystemState. Specifically, it more clearly separates adding/removing cycles to the balance from refunding/consuming cycles which on top of the balance updates the respective consumed metrics as well.

The idea is to separate in two pairs of APIs that will be responsible for each type of update:

fn add_cycles(amount: Cycles);
fn remove_cycles(amount: Cycles);

to handle balance only changes while

fn refund_cycles<T>(prepayment: CompoundCycles<T>, refund: CompoundCycles<T>);
fn consume_cycles<T>(amount: CompoundCycles<T>);

will handle metrics being updated as well.

This change has a two fold benefit.

On one hand, it allows us to perform direct updates to balance (e.g. when depositing cycles or attaching cycles in calls) without having to construct CompoundCycles<NonConsumed> which was a somewhat hacky way to say "no need to update metrics". In fact, after this change, NonConsumed can be completely removed from CyclesUseCases. This allows for some simplification in call sites where they don't need to deal with CompoundCycles or doing so incurs unnecessary extra work.

On the other, it allows us to require a prepayment for refunding cycles which would be very helpful in a follow up where a metric which acts as a counter will be introduced and the amount consumed will need to be computed based on prepayment and refund. This way the call sites that don't need or should not deal with prepayments (i.e. ones that need only add_cycles), they don't have to be exposed to any of this.

The meat of the changes are in SystemState while there are some updates in production code and tests to match the refined API. In many cases, it ends up in a simpler version of the code as a bunch of CompoundCycles::new calls are eliminated or the cost schedule does not need to be piped through various calls.

@dsarlis dsarlis requested a review from a team as a code owner April 2, 2026 11:42
@cgundy cgundy added the security-review-passed IDX or InfraSec have concluded it's safe to run CI on the external PR. label Apr 2, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 2, 2026

@cgundy
Copy link
Copy Markdown
Contributor

cgundy commented Apr 2, 2026

Seeing this error:

Clippy violations found!

To automatically fix many of these, run:

    cargo clippy --fix --locked --all-features --workspace --all-targets --keep-going -- --deny warnings --deny clippy::all --deny clippy::mem_forget --deny clippy::unseparated_literal_suffix --allow clippy::uninlined_format_args

Comment thread rs/replicated_state/src/canister_state/system_state.rs Outdated
Comment thread rs/replicated_state/src/canister_state/system_state.rs
Comment thread rs/replicated_state/src/canister_state/system_state.rs Outdated
Comment thread rs/replicated_state/src/canister_state/system_state.rs
Comment thread rs/replicated_state/src/canister_state/system_state.rs
@dsarlis dsarlis requested a review from mraszyk April 7, 2026 09:24
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

@mraszyk mraszyk enabled auto-merge April 7, 2026 09:30
@mraszyk mraszyk added this pull request to the merge queue Apr 7, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 7, 2026
@michael-weigelt michael-weigelt added this pull request to the merge queue Apr 7, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 7, 2026
@dsarlis
Copy link
Copy Markdown
Contributor Author

dsarlis commented Apr 7, 2026

@mraszyk Seems something failed on the merge queue twice. Unclear to me if the timeout is a real problem or not. It is plausible my changes are bringing the time over as I'm adding a few extra debug_asserts but for that to be true the runtime of that test should have been pretty close to the limit already. Can you or someone else look into it a bit and let me know if I need to do something from my side?

@mraszyk mraszyk added this pull request to the merge queue Apr 7, 2026
@mraszyk
Copy link
Copy Markdown
Contributor

mraszyk commented Apr 7, 2026

@dsarlis arm64-linux is often flaky so I just added this PR to the merge queue again...

@dsarlis
Copy link
Copy Markdown
Contributor Author

dsarlis commented Apr 7, 2026

It's arm64-darwin that failed, I assume that's the one you meant.

EDIT: the second time, first time arm64-linux failed as you said.

Merged via the queue into dfinity:master with commit 5f1f1c2 Apr 7, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contributor refactor security-review-passed IDX or InfraSec have concluded it's safe to run CI on the external PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants